Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add verify_signature function for secp curves #4120

Merged

Conversation

ArielElp
Copy link
Collaborator

@ArielElp ArielElp commented Sep 21, 2023

This change is Reviewable

@ArielElp ArielElp requested a review from orizi September 21, 2023 14:50
@ilyalesokhin-starkware
Copy link
Contributor

corelib/src/starknet/secp256_trait.cairo line 83 at r1 (raw file):

    let (x, y) = sum.get_coordinates().unwrap_syscall();
    return Result::Ok(x == r);

I find this a bit strange.

why not
Result::Err('Invalid signature')

also, no need to return here

Suggestion:

Result::Ok(x == r);

Copy link
Contributor

@ilyalesokhin-starkware ilyalesokhin-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all commit messages.
Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @ArielElp and @orizi)


corelib/src/starknet/secp256r1.cairo line 44 at r1 (raw file):

        } else {
            return secp256r1_get_point_from_x_syscall(x, true);
        }

Suggestion:

secp256_ec_new_syscall(x, y)

Copy link
Collaborator Author

@ArielElp ArielElp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware and @orizi)


corelib/src/starknet/secp256_trait.cairo line 83 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

I find this a bit strange.

why not
Result::Err('Invalid signature')

also, no need to return here

renamed to is_valid_signature, now returns bool


corelib/src/starknet/secp256r1.cairo line 44 at r1 (raw file):

        } else {
            return secp256r1_get_point_from_x_syscall(x, true);
        }

Done.

Copy link
Contributor

@ilyalesokhin-starkware ilyalesokhin-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArielElp and @orizi)


corelib/src/starknet/secp256k1.cairo line 82 at r2 (raw file):

) -> SyscallResult<(u256, u256)> implicits(GasBuiltin, System) nopanic;

/// Asserts that a Secp256 ECDSA signature is valid.

Fix the comment.

Code quote:

// Asserts that a Secp256 ECDSA signature is valid.

corelib/src/starknet/secp256k1.cairo line 92 at r2 (raw file):

}

/// Checks a Secp256 ECDSA signature.

fix

Code quote:

 Checks a Secp256 ECDSA signature.

corelib/src/starknet/secp256k1.cairo line 96 at r2 (raw file):

/// where N is the size of the curve.
/// Returns a Result with an error string if the signature is invalid.
fn is_eth_signature_valid(

consider moving the etherium related logic to eth.cairo.

Code quote:

is_eth_signature_valid

Copy link
Collaborator Author

@ArielElp ArielElp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 5 files at r2, 2 of 4 files at r3.
Reviewable status: 5 of 7 files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware and @orizi)


corelib/src/starknet/secp256k1.cairo line 82 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

Fix the comment.

Done.


corelib/src/starknet/secp256k1.cairo line 92 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

fix

Done.


corelib/src/starknet/secp256k1.cairo line 96 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

consider moving the etherium related logic to eth.cairo.

Moved to ethereum_signature.cairo (ideally we'd have eth.cairo with both address and signature but I don't want to move eth address as well, so IMO it's good enough)

Copy link
Contributor

@ilyalesokhin-starkware ilyalesokhin-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @orizi)

@ilyalesokhin-starkware ilyalesokhin-starkware added this pull request to the merge queue Oct 22, 2023
Merged via the queue into starkware-libs:main with commit df22d08 Oct 22, 2023
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants